Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: APP-322 terassos project filters UI #2487

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

wgardiner
Copy link
Contributor

@wgardiner wgardiner commented Sep 30, 2024

Description

Closes: APP-322

This PR adds UI for Terassos project filters and an approach for customizing color SVG colors


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. Review Organisms/ProjectFilters in Storybook deploy preview

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Sep 30, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit a77969d
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/670d31571ffd410008473751
😎 Deploy Preview https://deploy-preview-2487--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wgardiner wgardiner force-pushed the feat-APP-322-terassos-project-filters-ui branch from 15faeb1 to 5383368 Compare September 30, 2024 00:35
@blushi
Copy link
Member

blushi commented Sep 30, 2024

@wgardiner you can use the draft PR feature from github here if you think that's still a draft

@wgardiner wgardiner marked this pull request as draft October 2, 2024 00:12
@wgardiner wgardiner force-pushed the feat-APP-322-terassos-project-filters-ui branch from b699b7b to 26002ca Compare October 6, 2024 18:52
@wgardiner wgardiner marked this pull request as ready for review October 6, 2024 18:59
@wgardiner wgardiner changed the title Draft: Feat app 322 terassos project filters UI Feat app 322 terassos project filters UI Oct 7, 2024
@blushi blushi changed the title Feat app 322 terassos project filters UI feat: APP-322 terassos project filters UI Oct 8, 2024
Copy link
Collaborator

@flagrede flagrede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work on this PR, the UI looks nice!

Left a few comments, the biggest thing might be that the PR heavily rely on MUI instead of tailwind. We want to get rid of MUI in the long term so new components should not introduce new usage of MUI.
If this is too much of a pain to rewrite it for this PR, it's ok to leave it as it is but for new components it would be best to favor tailwind usage.

Comment on lines 5 to 19
<Box
display="flex"
alignItems="center"
justifyContent="center"
width="24px"
height="24px"
padding="3px"
borderRadius="50%"
fontSize="12px"
fontWeight={700}
sx={{
borderRadius: '40px',
border: '1px dashed var(--surface-stroke, #D2D5D9)',
background: 'var(--surface-selected-item-background, #EFEFEF)',
ml: 2,
}}
// eslint-disable-next-line lingui/no-unlocalized-strings
>
ha.
</Box>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use Tailwind instead of MUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved border radius and margin to use tailwind, but in this PR I don't yet have the Tailwind variables that I need to use here, so I can update it after we get the design tokens PR merged.

@wgardiner wgardiner force-pushed the feat-APP-322-terassos-project-filters-ui branch from e7d1d2f to 8d49167 Compare October 14, 2024 04:27
@wgardiner wgardiner force-pushed the feat-APP-322-terassos-project-filters-ui branch from 8d49167 to a77969d Compare October 14, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants